Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expand extractSymbol ranges #42770

Merged
merged 6 commits into from
Feb 17, 2021
Merged

Expand extractSymbol ranges #42770

merged 6 commits into from
Feb 17, 2021

Conversation

jessetrinity
Copy link
Contributor

@jessetrinity jessetrinity commented Feb 11, 2021 •
edited
Loading

Fixes #41469

When a refactor is invoked through a keyboard command we search a bit harder for the extract symbol refactor by treating the selected span as though it covers the tokens containing the start and end tokens.

That is, selecting

const foo = ba[|r + b|]az;

will be equivalent to selecting

const foo = [|bar + baz|];

for the purposes of offering the refactor to extract bar + baz.

I also separated out the unit tests into individual tests so we can tell them apart on failure.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Feb 11, 2021
@jessetrinity jessetrinity changed the title Expand extract ranges Expand extract symbol ranges Feb 11, 2021
@jessetrinity jessetrinity changed the title Expand extract symbol ranges Expand extractSymbol ranges Feb 11, 2021
@jessetrinity
Copy link
Contributor Author

cc @mjbvz

@amcasey
Copy link
Member

amcasey commented Feb 16, 2021

Does this resolve #41469 or is it just related? That issue seems to be about doing this for all refactorings.

amcasey
amcasey suggested changes Feb 16, 2021
View reviewed changes
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly questions.

src/services/refactors/extractSymbol.ts Show resolved Hide resolved
src/services/refactors/extractSymbol.ts Outdated Show resolved Hide resolved
src/services/refactors/extractSymbol.ts Show resolved Hide resolved
src/services/refactors/extractSymbol.ts Show resolved Hide resolved
src/testRunner/unittests/services/extract/ranges.ts Show resolved Hide resolved
@jessetrinity
Copy link
Contributor Author

jessetrinity commented Feb 16, 2021 •
edited
Loading

I expect that #41469 only relates to extractSymbol. Other refactorings generally only use the start of the span to search for an applicable refactor, so the "explicit cursor request" from #38378 should suffice. extractSymbol relies more on the start and end nodes both being in the right places so it needed special logic to "try harder".

amcasey
amcasey approved these changes Feb 16, 2021
View reviewed changes
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jessetrinity jessetrinity merged commit d640313 into microsoft:master Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amcasey amcasey amcasey approved these changes

@PranavSenthilnathan PranavSenthilnathan Awaiting requested review from PranavSenthilnathan

Assignees

@jessetrinity jessetrinity

Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Not seeing refactorings try to intelligently expand based on the current selection
3 participants
@jessetrinity @amcasey @typescript-bot